Generic Enumerable.Range method#106925
Conversation
ghost
added
|
Note regarding the |
1 similar comment
|
Note regarding the |
dotnet-policy-service
Bot
added
the
community-contribution
|
|
||
| public static IEnumerable<T> Range<T>(T start, int count) where T : IBinaryInteger<T> | ||
| { | ||
| if (count < 0) |
There was a problem hiding this comment.
Curious why this overload is using somewhat different validation flow compared to the existing method? Is it anticipating that T.CreateTruncating might be expensive?
| //public static TheoryData<T, int> StartCountIncorrectData; | ||
|
|
||
| [Theory] | ||
| [MemberData("StartCountCorrectData")] |
|
|
||
| internal static int StartMaxCount(T start, T max) | ||
| { | ||
| int count = int.CreateTruncating(max - start); |
There was a problem hiding this comment.
What happens if start is 0L and max is long.MaxValue in this case?
| _state = -1; // Don't reset current | ||
| } | ||
|
|
||
| internal static int StartMaxCount(T start, T max) |
There was a problem hiding this comment.
I'm guessing this is effectively trying to measure a distance between start and max? I'm not sure if this is necessarily a well-defined operation for every kind of binary integer type (e.g. Gaussian integers)
| }; | ||
| } | ||
|
|
||
| public class RangeBigIntegerTests : RangeIBinaryIntegerTests<BigInteger> |
There was a problem hiding this comment.
It would be interesting to see a few nonstandard numeric types that stress test the implementation, e.g. a modular arithmetic type. If for example I define a type representing all numbers modulo 7, what should be the behaviour of the new method when I do Range(Modulo7.Zero, 8)?
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
The build errors are caused by an expected source breaking change that this new API is introducing. The impacted call sites should be updated with an explicit int type parameter.
It is expected source breaking change that this new API is introducing.
stephentoub
added
breaking-change
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Marking as changes requested since there are a few unresolved questions on a number of corner cases.
|
Now that #111685 has been merged, we should augment this PR to add the corresponding generic Range method to AsyncEnumerable as well. In general, we want to keep the APIs consistent across Enumerable and AsyncEnumerable. |
…nge_IBinaryInteger
Updated usage of Enumerable.Range to specify <int> type parameter in VirtualDriveHelper.Windows.cs, ReadOnlySequenceFactory.char.cs, and ArrayTests.cs. This change improves code clarity and enhances type safety in drive letter generation and ASCII range handling, as well as strengthens assertions in unit tests.
Introduces a new method `Range<T>(T start, int count)` in the `AsyncEnumerable` class for generating sequences of integral numbers for types implementing the `IBinaryInteger<T>` interface. This method is conditionally compiled for .NET 7.0 or greater and includes validation for the `count` parameter. Updates `RangeTests` to cover various input scenarios for the new method, including edge cases for different integral types like `byte` and `long`. Additionally, enhances documentation for the existing `Range(int start, int count)` method.
@stephentoub Done! |
| for (int i = 0; i < count; i++, start++) | ||
| { | ||
| yield return start; | ||
| } |
There was a problem hiding this comment.
@tannergooding, is this the right implementation? The above non-generic does:
for (int i = 0; i < count; i++)
{
yield return start + i;
}Is this proposed implementation better or worse than:
for (int i = 0; i < count; i++)
{
yield return start + T.CreateTruncating(i);
}? I'm not sure what requirements we place on IBinaryInteger, in terms of guarantees we make about ++ actually incrementing the value (as opposed to, say, a float + 1 which might not actually increment).
| if (count < 0) | ||
| count = int.CreateTruncating(max) - int.CreateTruncating(start); | ||
| return count; | ||
| } |
There was a problem hiding this comment.
@tannergooding, is this how we'd recommend doing overflow checking?
This commit introduces a new implementation of the `Range` method for `IAsyncEnumerable<T>` that supports `IBinaryInteger<T>` types, specifically for .NET Core. The method generates a sequence of integral numbers within a specified range, with input validation and exception handling for out-of-range values. Project files have been updated for conditional compilation targeting .NET Core, and the previous `Range` method for non-`IBinaryInteger<T>` types has been removed. Tests have also been updated to reflect these changes.
- Introduced `System.Numerics` for `BigInteger` usage. - Added test for `ArgumentOutOfRangeException` with `BigInteger` in `InvalidInputs_Throws`. - Implemented tests for various `BigInteger` values in `VariousValues_MatchesEnumerable`.
Modified `RangeTests.cs` to test a wider range of `BigInteger` values instead of just `long`. Added a new test case for `AsyncEnumerable.Range<BigInteger>` that checks the behavior when skipping and taking elements from a large range. All tests executed successfully with no errors or failures.
Modified test cases in `InvalidInputs_Throws` in `RangeTests.cs` to validate `ArgumentOutOfRangeException` for negative count values in `AsyncEnumerable.Range` for `byte`, `long`, and `BigInteger` types. All tests passed successfully.
Updated assertions in `Array_ByValueOut` and `Array_ByValueOut_This` methods to cast characters in `testArray` to integers. This ensures proper comparison with the expected output of `Enumerable.Range<int>`, enhancing test accuracy.
|
@AlexRadch, thanks for working on this. It's really challenging to implement the agreed upon signature ( |
jeffhandley
removed
the
needs-breaking-change-doc-created
5 participants
Close #97689